Skip to content

feat: Add America/Coyhaique timezone support#16362

Open
n0r0shi wants to merge 1 commit intofacebookincubator:mainfrom
n0r0shi:add-timezone-america-coyhaique
Open

feat: Add America/Coyhaique timezone support#16362
n0r0shi wants to merge 1 commit intofacebookincubator:mainfrom
n0r0shi:add-timezone-america-coyhaique

Conversation

@n0r0shi
Copy link
Copy Markdown
Contributor

@n0r0shi n0r0shi commented Feb 12, 2026 •
edited
Loading

Summary

  • Add America/Coyhaique (timezone ID 2234) to TimeZoneDatabase.cpp, following the Presto zone-index convention
  • Add display names to TimeZoneNames.cpp (Coyhaique Standard Time, Coyhaique Daylight Time)
  • Fix namespace in gen_timezone_database.py (facebook::velox::util → facebook::velox::tz)
  • Add test coverage for the new timezone entry

America/Coyhaique was added to the IANA timezone database in tzdata 2025b (JDK 17+). Systems running newer tzdata include this zone, but Velox rejects it because it's not in the hardcoded timezone ID map, causing queries with session_timezone = 'America/Coyhaique' to fail.

The upstream Presto PR (prestodb/presto#26981) adding this zone to zone-index.properties has been merged.

How the files were generated

TimeZoneDatabase.cpp — generated from the Presto zone-index.properties:

./velox/type/tz/gen_timezone_database.py \
  -f /path/to/zone-index.properties \
  > velox/type/tz/TimeZoneDatabase.cpp

TimeZoneNames.cpp — short and long names obtained from Java's DateFormatSymbols (openjdk 17):

javac velox/type/tz/GenTimeZoneNames.java
java velox.type.tz.GenTimeZoneNames > velox/type/tz/TimeZoneNames.cpp

The entry was added manually to avoid a noisy full-file diff from formatting differences.

Related

Test plan

  • getTimeZoneName(2234) returns "America/Coyhaique"
  • getTimeZoneID("America/Coyhaique") returns 2234
  • Generated TimeZoneDatabase.cpp matches committed file exactly

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 12, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 12, 2026 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit c82e833
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69ac01b6ea91af0009ed1084

@n0r0shi n0r0shi mentioned this pull request Feb 12, 2026
Open
@n0r0shi n0r0shi force-pushed the add-timezone-america-coyhaique branch from 5dd591e to 2ae9467 Compare February 12, 2026 09:40
@kgpai
Copy link
Copy Markdown
Contributor

kgpai commented Feb 12, 2026

Hi @n0r0shi The Timezonedatabase file is autogenerated and based on what is supported by Presto. We can make this change after prestodb/presto#26981 lands in Presto.

@n0r0shi
Copy link
Copy Markdown
Contributor Author

n0r0shi commented Feb 14, 2026

@kgpai Thanks for the context. I understand that prestodb/presto#26981 needs to be merged first since Velox pulls its timezone database from Presto. That's exactly why I've kept this PR as a draft. I'm happy to wait until the Presto change lands, and I'll rebase and mark it ready for review once it does.

@n0r0shi
feat: Add America/Coyhaique timezone support
c82e833 
Add America/Coyhaique (timezone ID 2234) to the timezone database and
timezone names, following the Presto zone-index convention. Also fix
the namespace in gen_timezone_database.py (util -> tz) to match the
current codebase.

America/Coyhaique was added to the IANA timezone database in tzdata
2025b. The upstream Presto PR (prestodb/presto#26981) has been merged.

Fixes facebookincubator#16216
@n0r0shi n0r0shi force-pushed the add-timezone-america-coyhaique branch from 2ae9467 to c82e833 Compare March 7, 2026 10:45
@n0r0shi n0r0shi marked this pull request as ready for review March 7, 2026 10:48
@n0r0shi
Copy link
Copy Markdown
Contributor Author

n0r0shi commented Mar 7, 2026 •
edited
Loading

Updated this PR — it's no longer a draft since the upstream Presto PR (prestodb/presto#26981) has been merged.

Changes since the original version:

  • Added America/Coyhaique display names to TimeZoneNames.cpp
  • Fixed namespace in gen_timezone_database.py (util → tz) to match the current codebase

Ready for review. @rui-mo @zhli1142015 @kgpai

rui-mo
rui-mo reviewed Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n0r0shi It looks like the Presto version currently used in the fuzzer tests doesn’t include this change yet, which is causing the below failure. You may need to skip this timezone in the fuzzer tests until the Presto version is updated.

E20260307 03:40:21.303088   450 Exceptions.h:53] Line: /__w/velox/velox/velox/velox/exec/fuzzer/PrestoQueryRunner.cpp:119, Function:throwIfFailed, Expression:  Presto query failed: 7 Invalid format: "36037-07-22 16:57:47.864 America/Coyhaique" is malformed at "America/Coyhaique", Source: RUNTIME, ErrorCode: INVALID_STATE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rui-mo rui-mo rui-mo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Velox rejects valid IANA timezone America/Coyhaique present in system tzdb

3 participants

@n0r0shi @kgpai @rui-mo